Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure invalid username/password returns 401 error, not 403 #41775

Merged
merged 1 commit into from
Apr 8, 2017
Merged

Ensure invalid username/password returns 401 error, not 403 #41775

merged 1 commit into from
Apr 8, 2017

Conversation

CaoShuFeng
Copy link
Contributor

If a user attempts to use basic auth, and the username/password combination
is rejected, the authenticator should return an error. This distinguishes
requests that did not provide username/passwrod (and are unauthenticated
without error) from ones that attempted to, and failed.

Related to:
#39408

@k8s-ci-robot
Copy link
Contributor

Hi @CaoShuFeng. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Feb 21, 2017
@k8s-ci-robot
Copy link
Contributor

@CaoShuFeng: you can't request testing unless you are a kubernetes member.

In response to this comment:

@k8s-bot ok to test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@CaoShuFeng CaoShuFeng mentioned this pull request Feb 21, 2017
Merged
@liggitt liggitt self-assigned this Feb 21, 2017
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 21, 2017
@liggitt
Copy link
Member

liggitt commented Feb 21, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2017
@liggitt
Copy link
Member

liggitt commented Feb 22, 2017

/approve

@liggitt
Copy link
Member

liggitt commented Feb 22, 2017

looks like lint is unhappy with that var name:

W0221 12:24:35.513] staging/src/k8s.io/apiserver/plugin/pkg/authenticator/request/basicauth/basicauth.go:37:5: error var invalidAuth should have name of the form errFoo
W0221 12:24:35.513] 
W0221 12:24:35.514] Please fix the above errors. You can test via "golint" and commit the result.
W0221 12:24:35.514] 
I0221 12:24:35.614] FAILED   hack/make-rules/../../hack/verify-golint.sh	46s

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
@CaoShuFeng
Copy link
Contributor Author

CaoShuFeng commented Feb 22, 2017 •
edited

@liggitt I have updated the var name. Thanks for pointing it out.

@CaoShuFeng
Copy link
Contributor Author

CaoShuFeng commented Feb 22, 2017 •
edited

@k8s-merge-robot becomes crazy to comment this pr.

@CaoShuFeng
Ensure invalid username/password returns 401 error, not 403
0ec585c 
If a user attempts to use basic auth, and the username/password combination
is rejected, the authenticator should return an error. This distinguishes
requests that did not provide username/passwrod (and are unauthenticated
without error) from ones that attempted to, and failed.
@liggitt
Copy link
Member

liggitt commented Feb 22, 2017

@apelisse the bot has deleted and added the "not approved" comment about 200 times in the last few hours. Did a bad update just roll out?

@apelisse
Copy link
Member

@liggitt Look like it stopped?

@apelisse apelisse mentioned this pull request Feb 22, 2017
Closed
@liggitt
Copy link
Member

liggitt commented Feb 23, 2017

@k8s-bot ok to test

@liggitt liggitt added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Mar 8, 2017
@liggitt
Copy link
Member

liggitt commented Apr 1, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CaoShuFeng, liggitt
We suggest the following additional approver: @sttts

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@CaoShuFeng
Copy link
Contributor Author

@k8s-bot non-cri e2e test this

@CaoShuFeng
Copy link
Contributor Author

Hi, @deads2k can you add add an approve label for this patch?

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7d4fe5f into kubernetes:master Apr 8, 2017
@CaoShuFeng CaoShuFeng deleted the username_password_401_403 branch April 14, 2017 09:11
@liggitt liggitt mentioned this pull request Jun 9, 2017
Closed
@petervo petervo mentioned this pull request Jun 16, 2017
Closed
This was referenced Jul 31, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

@vmarmol vmarmol

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@CaoShuFeng @k8s-ci-robot @k8s-reviewable @liggitt @apelisse @k8s-github-robot @vmarmol